PDX-0: fix(mcp): address PR #145 review comments for security hardening#146
Conversation
RCA: Copilot review on PR #145 identified two security gaps: (1) pathPolicy.ts did not strip trailing separators from allowed-root keys, causing double-sep prefix checks ("/tmp//") and equality mismatches that incorrectly reject valid child paths when users pass roots like "/tmp/"; (2) testPlanTools.ts accepted absolute test_case_path values, allowing path.join(projectRoot, absolutePath) to silently discard projectRoot on Windows (drive-letter) and Unix, enabling reads outside the project. Fix: Strip trailing path.sep from allowed-root keys in pathPolicy (except filesystem roots "/" and "C:\"). Reject absolute test_case_path values in add-instance handler before path.join. Add two new tests: trailing-sep cases in pathPolicy.test.ts and an absolute-path rejection in testPlanTools.test.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens MCP path handling based on prior review feedback by normalizing allowed roots in assertPathAllowed and preventing absolute test_case_path inputs from escaping the project root.
Changes:
- Strip trailing path separators from allowed-root keys in
assertPathAllowed(while attempting to preserve filesystem roots). - Reject absolute
test_case_pathvalues inprovar_testplan_add-instancebefore any joining/normalization. - Add unit tests for trailing-separator allowed roots and absolute-path rejection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/mcp/security/pathPolicy.ts |
Normalizes allowed-root keys to avoid double-separator prefix checks and equality mismatches. |
src/mcp/tools/testPlanTools.ts |
Blocks absolute test_case_path inputs to prevent escaping project_path via path.join. |
test/unit/mcp/pathPolicy.test.ts |
Adds coverage for allowed roots specified with trailing separators. |
test/unit/mcp/testPlanTools.test.ts |
Adds coverage for rejecting absolute test_case_path values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // checks ("startsWith('/tmp//')") and equality mismatches ("/tmp" !== "/tmp/"). | ||
| const isRoot = rawBaseKey === path.sep || (isWindows && /^[a-z]:[/\\]$/.test(rawBaseKey)); | ||
| const baseKey = !isRoot && rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; | ||
| return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); |
There was a problem hiding this comment.
Fixed in 9b5f32d. Root base keys are now handled separately: when isRoot is true, the code calls resolvedKey.startsWith(rawBaseKey) directly since the root already ends with its own separator. Non-roots continue to use the boundary check (resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep)). Also added a regression test using path.parse(tmp).root that covers both Unix / and Windows C:.
| @@ -44,6 +44,14 @@ describe('pathPolicy', () => { | |||
| assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'a', 'b', 'c', 'file.xml'), [tmp])); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Added in 9b5f32d. New test 'allows child paths when allowed root is the filesystem root' uses path.parse(tmp).root to get the platform root (/ on Unix, C:\ on Windows) and asserts that a child of os.tmpdir() is allowed when the filesystem root is the only entry in allowedPaths. This directly catches the startsWith('//') / startsWith('C:') regression.
| const result = server.call('provar_testplan_add-instance', { | ||
| project_path: projectDir, | ||
| test_case_path: path.join(projectDir, 'tests', 'MyTest.testcase'), | ||
| plan_name: 'MyPlan', | ||
| overwrite: false, | ||
| dry_run: false, | ||
| }); |
There was a problem hiding this comment.
Fixed in 9b5f32d. The test now derives the absolute path from the filesystem root: path.join(path.parse(projectDir).root, 'escape', 'MyTest.testcase'). This produces C:\escape\MyTest.testcase on Windows or /escape/MyTest.testcase on Unix, guaranteed to be absolute regardless of how projectDir is constructed.
…st robustness RCA: PR #146 Copilot review found three issues in the previous security fix: (1) filesystem root allowed-paths (/ or C:\) were broken — appending path.sep to a root key produces // or C:\ so startsWith always fails for children, meaning only the root itself was allowed instead of everything under it; (2) no regression test existed for the filesystem-root case; (3) the absolute-path rejection test relied on projectDir being absolute rather than constructing the absolute path explicitly. Fix: Handle root base keys separately in assertPathAllowed — use startsWith(rawBaseKey) directly since the root already ends with its own separator. Add a filesystem-root regression test using path.parse(tmp).root. Rewrite the absolute-path test to derive the escape path from path.parse(projectDir).root so it is always absolute regardless of how projectDir is constructed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
RCA: Copilot review on PR #145 identified two security gaps: (1) pathPolicy.ts did not strip trailing separators from allowed-root keys, causing double-sep prefix checks ("/tmp//") and equality mismatches that incorrectly reject valid child paths when users pass roots like "/tmp/"; (2) testPlanTools.ts accepted absolute test_case_path values, allowing path.join(projectRoot, absolutePath) to silently discard projectRoot on Windows (drive-letter) and Unix, enabling reads outside the project.
Fix: Strip trailing path.sep from allowed-root keys in pathPolicy (except filesystem roots "/" and "C:"). Reject absolute test_case_path values in add-instance handler before path.join. Add two new tests: trailing-sep cases in pathPolicy.test.ts and an absolute-path rejection in testPlanTools.test.ts.